Log why request-handler unregistration fails during teardown by rolfheij-sil · Pull Request #2573 · paranext/paranext-core

rolfheij-sil · 2026-07-16T19:56:38Z

Summary

At every app quit, the log shows an anonymous failure with zero context:

UnsubscriberAsyncList platformScriptureEditor: Unsubscriber at index 19 failed!

Diagnosis: the failing unsubscriber is a registerCommand unregistration that
resolves false rather than throwing, and nothing in the chain logs why.
The chain:

src/shared/services/network.service.ts — the unsubscriber closure returned
by registerRequestHandler returns false if jsonRpc is unset, or passes
through whatever jsonRpc.unregisterMethod(requestType) resolves.
src/client/services/rpc-client.ts unregisterMethod has three silent
false-returning branches: the method isn't locally registered (checked
twice, once before and once after acquiring the mutex), or the remote
UNREGISTER_METHOD round-trip itself resolved false.

None of these branches logged anything, so UnsubscriberAsyncList only ever
saw an opaque false with no way to tell which branch fired or for which
request/method.

Changes

Added one logger.warn per silent branch, each naming the request/method
type so the log line is actionable:

network.service.ts: warn when the closure returns false because
jsonRpc is unset, and warn if jsonRpc.unregisterMethod itself resolved
false.
rpc-client.ts unregisterMethod: warn (matching this file's existing
logger.warn convention for the analogous registerMethod "already
registered" branches) distinguishing "not locally registered" from "remote
unregister round-trip failed", each naming the methodName.

Behavior is unchanged — return values, ordering, and error propagation
are identical; this is purely additive logging. The underlying failure is
pre-existing and cosmetic (nothing depends on the unsubscriber succeeding at
quit-time); this PR just makes the next occurrence self-describing instead of
an anonymous index number.

Test plan

eslint on both changed files — clean
tsc -p ./tsconfig.json --noEmit — no new errors (the one pre-existing
error, a missing generated release/app/buildInfo.json, reproduces
identically on unmodified main and is unrelated to these files)
vitest run src/shared/services/__tests__/network.service.shared-events.test.ts
— 10/10 passing (no dedicated unit tests exist for rpc-client.ts)

🤖 Generated with Claude Code

Self-review — 2026-07-17 (review-paratext methodology)

Approve as-is — clean, low-risk, additive-only diagnostic logging; no behavior change. Four analysis passes (api / style / compliance / ux) per .claude/agents/review-analyzer.md, run by a review-lead agent; author interview substituted with the PR body + PRD design records; every finding adversarially verified against code and tests.

#	severity	finding	disposition
1	minor	`unregisterMethod`'s pre-mutex fast-path and post-mutex re-check log the identical message, so a log line alone can't distinguish "already gone" from "lost the race"	accepted (mirrors existing `registerMethod` precedent)

Investigated separately: the "could this spam every shutdown" concern is unfounded — the new warn logs only fire on paths where two more-severe, pre-existing logs (console.error in UnsubscriberAsyncList, logger.error in deactivateExtension) already fire today.

Gates: prettier --check pass, tsc --noEmit 0 errors, eslint 0 errors/warnings, vitest network.service.shared-events.test.ts 10/10 pass.

This change is

At app quit, UnsubscriberAsyncList logs a bare "Unsubscriber at index N failed!" with no context when a registerCommand unregistration resolves false instead of throwing. Add logger.warn calls at each silent false-returning branch (jsonRpc unset in the network.service closure; method not locally registered or remote UNREGISTER_METHOD round-trip failed in rpc-client's unregisterMethod) so the next occurrence self-describes. No behavior change — return values, ordering, and error propagation are identical. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

lyonsil · 2026-07-18T00:03:32Z

Code review

Found 1 issue:

The new logger.warn on the !jsonRpc branch fires on every graceful shutdown, which is a documented, expected no-op rather than a failure. shutdown() tears down the handler reference first so disposers "skip the now-pointless per-event unregister call — the whole connection is already going away," and the sibling disposeNetworkEventEmitter returns silently for the same !jsonRpc condition. Warning here adds spurious noise on normal quit, working against the PR's goal of making teardown logs meaningful. Consider debug level (or no log) for this branch, reserving warn for the genuine unregistered === false case just below it.

paranext-core/src/shared/services/network.service.ts

Lines 319 to 327 in 105fbb4

    
           return async () => { 
        
             if (!jsonRpc) { 
        
               logger.warn(`Could not unregister request handler for "${requestType}": jsonRpc is not set`); 
        
               return false; 
        
             } 
        
             removeTimeoutMsForRequestType(requestType); 
        
             const unregistered = await jsonRpc.unregisterMethod(requestType); 
        
             if (!unregistered) logger.warn(`Failed to unregister request handler for "${requestType}"`); 
        
             return unregistered;

Documented expected-quiet-shutdown intent it contradicts:

paranext-core/src/shared/services/network.service.ts

Lines 112 to 115 in 105fbb4

    
           await jsonRpc.disconnect(); 
        
           // Tear down the handler reference before disposing emitters so their disposers skip the 
        
           // now-pointless per-event unregister call — the whole connection is already going away. 
        
           jsonRpc = undefined;

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

… of warn Addresses lyonsil's #2573 review: the !jsonRpc branch is the normal graceful-shutdown path (shutdown() clears jsonRpc before disposing emitters), so warning there fires on every quit. Drop to debug; keep warn for the genuine unregistered === false case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

timothy-mccormack · 2026-07-21T18:06:33Z

🤖 Claude: Good catch — agreed. The !jsonRpc branch is the normal-shutdown path: shutdown() clears jsonRpc before disposing emitters precisely so their disposers skip the per-event unregister ("the whole connection is already going away"), so a warn there fires on every graceful quit — noise, and against this PR's goal of making teardown logs meaningful. Dropped it to logger.debug (a quiet diagnostic rather than fully silent, in the spirit of disposeNetworkEventEmitter's !jsonRpc early return), and kept logger.warn for the genuine unregistered === false case just below. Fixed in 0e394a0.

lyonsil

lgtm

lyonsil

@lyonsil reviewed 2 files and all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved.

rolfheij-sil requested review from irahopkinson, lyonsil and tjcouch-sil as code owners July 16, 2026 19:56

rolfheij-sil force-pushed the unregister-failure-logging branch from fb6f3f0 to 105fbb4 Compare July 17, 2026 15:40

lyonsil approved these changes Jul 21, 2026

View reviewed changes

timothy-mccormack enabled auto-merge (squash) July 21, 2026 18:28

timothy-mccormack merged commit 1d4125e into main Jul 21, 2026
7 checks passed

timothy-mccormack deleted the unregister-failure-logging branch July 21, 2026 18:39

Navigation Menu

Uh oh!

Log why request-handler unregistration fails during teardown - #2573

Log why request-handler unregistration fails during teardown#2573
timothy-mccormack merged 2 commits into
mainfrom
unregister-failure-logging

rolfheij-sil commented Jul 16, 2026 •

edited

Loading

Uh oh!

lyonsil commented Jul 18, 2026

Uh oh!

timothy-mccormack commented Jul 21, 2026

Uh oh!

lyonsil left a comment

Uh oh!

lyonsil left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

rolfheij-sil commented Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Test plan

Self-review — 2026-07-17 (review-paratext methodology)

Uh oh!

lyonsil commented Jul 18, 2026

Code review

Uh oh!

timothy-mccormack commented Jul 21, 2026

Uh oh!

lyonsil left a comment

Choose a reason for hiding this comment

Uh oh!

lyonsil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rolfheij-sil commented Jul 16, 2026 •

edited

Loading